[CALCITE-7825] RelMdTableReferences does not account for tables in RexSubQuery expressions or Correlate - #5298
Conversation
ehds
changed the title
| assertNull(tableReferences); | ||
| } | ||
|
|
||
| @Test void testTableReferencesValues() { |
There was a problem hiding this comment.
these tests are not easy for a human to review, I hope they are right
There was a problem hiding this comment.
These tests use SQL or RelBuilder to construct the different Rel types being tested, following the same approach as the existing tests.I've checked each plan and its expected table references.
| * Table references from TableFunctionScan. | ||
| * | ||
| * <p>Returns an empty set if there are no inputs, and {@code null} if the table | ||
| * references of any input cannot be determined. Tables accessed internally |
There was a problem hiding this comment.
why delete this comment?
There was a problem hiding this comment.
The shared behavior is already documented at the class level, and this PR also includes tables referenced by sub-queries in TableFunctionScan’s RexCall. Therefore, I simplified the comment to keep it consistent with the other methods.
|
|
||
| /** Table references from Snapshot. */ | ||
| public @Nullable Set<RelTableRef> getTableReferences(Snapshot rel, RelMetadataQuery mq) { | ||
| return mq.getTableReferences(rel.getInput()); |
There was a problem hiding this comment.
don't you need to analyze the period operand to?
There was a problem hiding this comment.
Yes, the period expression should also be inspected. For example, MariaDB allows a sub-query here. I've updated the handler accordingly.
|
|
||
| /** Table references from Match. */ | ||
| public @Nullable Set<RelTableRef> getTableReferences(Match rel, RelMetadataQuery mq) { | ||
| return mq.getTableReferences(rel.getInput()); |
There was a problem hiding this comment.
don't you need to analyze the pattern and measures operands?
There was a problem hiding this comment.
Yes, I've updated the handler to inspect the pattern, measures, and other RexNode expressions in Match as well.
| } | ||
|
|
||
| /** Table references from Spool. */ | ||
| public @Nullable Set<RelTableRef> getTableReferences(Spool rel, RelMetadataQuery mq) { |
There was a problem hiding this comment.
apparently there are still some classes not handled, like Window, Sort, TableModify
There was a problem hiding this comment.
Window, Sort, and TableModify already had handlers in the existing code.
| List<RelNode> inputs, RelMetadataQuery mq) { | ||
| List<RelNode> inputs, List<? extends RexNode> expressions, RelMetadataQuery mq) { | ||
| final List<RelNode> rels = new ArrayList<>(inputs); | ||
| final RexVisitorImpl<Void> visitor = new RexVisitorImpl<Void>(true) { |
There was a problem hiding this comment.
you can reuse the SubQueryCollector visitor
There was a problem hiding this comment.
The existing SubQueryCollector exposed only collect(Project), and its visitSubQuery stopped after recording a sub-query, so it could miss sub-queries nested in a sub-query’s operands.
It also had no callers in the current codebase. I updated it to accept a deep option and reused the visitor in RelMdTableReferences to scan each expression with deep=true.
|
|
Please squash the commits for merging. |
mihaibudiu
added
the
LGTM-will-merge-soon
…xSubQuery expressions or Correlate
08ff544 to
7392942
Compare
Done. |
2 participants
Jira Link
CALCITE-7825
This is the follow-up to the #5288. @zzwqqq @vlsi Could you please review these changes?
RelMdTableReferences misses tables in RexSubQuery expressions in Project, Filter, Join, and Calc. It also has no Correlate handler, so rewriting a sub-query can change the result to null.
Other nodes also return null despite having determinable table references: Values should return an empty set, unary nodes should pass through their input references, and RepeatUnion and Combine should merge theirs.
Changes Proposed
Extend RelMdTableReferences to cover sub-queries, Correlate, and other nodes that previously returned null despite having determinable table references. Refactor Join to use the shared references extractor.